-
Notifications
You must be signed in to change notification settings - Fork 25.6k
[ES|QL] RERANK command - Updating the syntax and behavior #129488
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
💚 CLA has been signed |
4aec0dc to
cc96232
Compare
cc96232 to
1fdb090
Compare
…rerank-refactoring
- Not sorting the data anymore - Output the score in the specified column - Applied additional logical plan optimizations since the plan is now a generating plan (because we removed the SORT clause) - Updated tests
f503e74 to
d388ff8
Compare
…rerank-refactoring
…rerank-refactoring
…rerank-refactoring
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/LogicalPlanBuilder.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/LogicalPlanBuilder.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/LogicalPlanBuilder.java
Outdated
Show resolved
Hide resolved
...main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PushDownAndCombineLimits.java
Show resolved
Hide resolved
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/LogicalPlanBuilder.java
Outdated
Show resolved
Hide resolved
...ck/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/LocalExecutionPlanner.java
Show resolved
Hide resolved
...ck/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/LocalExecutionPlanner.java
Show resolved
Hide resolved
…rerank-refactoring
…rerank-refactoring
72ff951 to
d86d16b
Compare
...ck/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/inference/Rerank.java
Show resolved
Hide resolved
| } | ||
|
|
||
| return refs.build(); | ||
| return Rerank.computeReferences(rerankFields); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to add the scoreAttribute here as well the way we had it before.
I am just looking at what we do in the Analyzer when we resolve the score attribute.
If the child plan has the score attribute, we reuse it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not really needed to consider the target field as a reference, no matter if it exists before or not.
Indeed it is not really a reference because its value is never used / read by the plan.
Eval, Grok and other Generating plans are doing exactly the same and are not putting output fields in references.
Hope it makes sense.
…rerank-refactoring
💔 Backport failed
You can use sqren/backport to manually backport by running |
💚 All backports created successfully
Questions ?Please refer to the Backport tool documentation |
…9488) (cherry picked from commit 34ccaba) # Conflicts: # x-pack/plugin/esql/qa/testFixtures/src/main/resources/rerank.csv-spec # x-pack/plugin/esql/src/main/antlr/EsqlBaseParser.g4 # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PushDownAndCombineFilters.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/EsqlBaseParser.interp # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/EsqlBaseParser.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/EsqlBaseParserBaseListener.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/EsqlBaseParserBaseVisitor.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/EsqlBaseParserListener.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/EsqlBaseParserVisitor.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/LogicalPlanBuilder.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/inference/Rerank.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/LocalExecutionPlanner.java # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/StatementParserTests.java
RERANKwill only compute scores for the provided rows. We let the user append aSORTcommand explicitly if he want the rows to be sortedRERANK<query_text>: The query<field>: Text field use as input for the rerank modelONis consistent with other command syntax (LOOKUP JOINandENRICHare using it to specify a field to join/enrich on)_scorecolumn (overriding it if it already exists).rerank-v1-elastic)Additional notes:
RERANKdoes not sort the documents anymore. This give us the ability to add all the optimizers rules that were added on theCOMPLETIONcommand toRERANKtoo (such inPushDownAndCombineLimits)Rerank command main issue: #124337